perf(rpc): stop storing the block header in every block record - #90
Open
rabbitson87 wants to merge 2 commits into
Open
perf(rpc): stop storing the block header in every block record#90rabbitson87 wants to merge 2 commits into
rabbitson87 wants to merge 2 commits into
Conversation
The node holds one `BlockRecord` per applied block for the life of the process and nothing removes one, so the record's own footprint is the cost. The `BlockTree` already holds a full header on every node, so the record stored it twice. Every constructor now leaves the field empty. `Context::header_record` is the only thing that produces a header, from the tree node it resolved - the tree becomes the single source of truth for what a block's header is. size_of::<BlockRecord>(): 168 -> 88 80 bytes per block, about 73.5 MiB at a mainnet tip on top of the 88 MiB the parent commit saved by storing raw bytes instead of hex. This is footprint, not time: no benchmark, and it should not be quoted as an observed RSS drop. `record_for_hash` step 1 resolves the tree node and then looks for a cached record with the same hash and height. It used to return that cached record verbatim, which would now answer with no header at all; it splices the tree header in instead. Costs no extra lock - `header_record` has already taken and released the tree guard, and the header it produced outlives it. The boxing is the saving, not a detail. `Option<[u8; 80]>` costs its full 80 bytes in every record even when `None`, so emptying the log's records while leaving the array inline would have saved nothing. The parent commit rejected `Option<Box<..>>` because it "lands at the same 168 bytes while keeping the per-block allocation" - true while every record carries a header, and no longer true once none of them do. Removing the field was a design question rather than a refactor because two fallbacks exist for "the tree does not know this hash". Neither is reachable in a running node: `apply_block` inserts the header into the tree before pushing the record through the same handles; the tree never drops a node (the only `Slab` operation is `insert`, and `invalidate_subtree` only flips status); the log is not durable, rebuilt empty on every open; and a checkpoint restore rebuilds the tree from genesis contiguously. Both fallbacks are left in place - removing an unreachable fallback is a separate claim from removing a stored field - but the REST one now yields an empty result, which a new test pins rather than leaving silent. Three `getblock` tests built a context from a record alone. They seed the tree too now, through a `seed_block` helper that does what `apply_block` does. A record on its own was never a node's state. Five mutations, all killed; baseline and restored green across 14 targets. See docs/benchmarks/block-record-footprint.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The parent commit stopped storing the block header in `BlockRecord` on the grounds that the block tree already has it by the time the record is in the log. That is an ordering: `applied_header_tip` inserts, then the record is pushed, through the same handles. Nothing checked it. The push site now asserts it. Reverse the two statements and every `getblock` / `getblockheader` answer for a freshly applied block loses its header, with nothing failing at the point the mistake is made. The tree lock is free there - `applied_header_tip` releases its write guard before returning - and the check is one hash-table lookup, compiled out of release builds. It is not a lone test. Moving the record push above the tree insert, which is exactly the mistake it defends against, fails 52 node tests, each on this assertion naming the block that would have lost its header. Every node test that applies a block now exercises the invariant. Also records, in docs/benchmarks/block-record-footprint.md, the one part of the safety argument that needed checking rather than assuming: a reorg calls `invalidate_subtree`, and had `lookup` filtered on node status then an invalidated block's header would have become unreachable while its record was still in the log. `lookup` matches on hash alone and `by_hash` is insert-only, so a hash resolves for the life of the process whatever happens to its branch. This remains weaker than Bitcoin Core, where `CBlockIndex` holds the header and the payload facts in one structure so "a record with no index entry" is not representable. Here they are two structures held in step by an ordering. Merging them is the Core-shaped end state and a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #86. Merge that first — this branch is based on it, and the diff
below is only the delta.
The node holds one
BlockRecordper applied block for the life of the processand nothing removes one, so the record's own footprint is the cost. The
BlockTreealready holds a fullbitcoin::block::Headeron every node — therecord stored it a second time.
String[u8; 80](#86)This is footprint, not time.
size_of::<BlockRecord>()× record count,pinned by a test — not an observed RSS drop, and it should not be quoted as one.
There is no benchmark and no harness in this workspace that attributes resident
memory to the block-record log.
Why this was a design question, not a refactor
#86 stopped short of removing the field, and the plan for it said why: two
fallbacks exist for "the tree does not know this hash" —
Context::record_for_hashstep 2 and the singleton inrest.rs— and theircomments describe it as a real state ("a block seen before a checkpoint
restore"). If it is real, dropping the field turns a working
getblock/getblockheader/ REST answer into an empty one.It is not reachable in a running node:
apply_blockinserts via
applied_header_tip(apply.rs:2338-2343) and pushes the recordafterwards (
:2354), through the sameArc<RwLock<BlockTree>>.Slaboperation isinsert(
chain/src/tree.rs:604);invalidate_subtreeflipsstatusand clears theheight index, never the slab. The
"pruned out of BlockTree"comment inundo_pruner.rsis about the redb column family, not this tree.state.rs:1061builds it empty on every open, so arecord cannot survive a restart into the state its own comment describes.
(
checkpoint.rs:242-329assertsnode.height == heightfrom index 0 up).Context::add_blockhas no non-test callers, and everyBlockRecord::from_block*outsideapplied_block_recordis a test or bench.Context::header_recordwas already the precedent — it builds a record whoseheader comes from
tree.node_by_hash(hash), an O(1) lookup.What changed
Every constructor leaves
header: None.header_recordis the only producer, sothe tree is the single source of truth for what a block's header is.
record_for_hashstep 1 resolves the tree node, then looks for a cached recordwith the same hash and height. It returned that cached record verbatim, which
would now answer with no header at all — it splices the tree's header in instead.
No extra lock:
header_recordhas already taken and released the tree guard,and the header it produced outlives it.
Consumers are unchanged — all four already read through
header_bytes()/header_hex(), which #86 introduced.The boxing is the saving, not a detail
Option<[u8; 80]>costs its full 80 bytes in every record even whenNone.Emptying the log's records while leaving the array inline would have saved
nothing at all.
#86 considered and rejected
Option<Box<[u8; 80]>>because it "lands at the same168 bytes total while keeping the per-block allocation". That was correct while
every record carries a header. It stops being correct once none of them do, which
is exactly what this change makes true: the box is 8 bytes in the log and
allocates once per RPC answer instead of once per block.
Behaviour that changed
rest.rs's singleton fallback has no header to serve. Reaching it meansthe tree has no node for the hash, so there is none to be had; it yields an
empty result. Code and comment left in place — removing an unreachable fallback
is a separate claim from removing a stored field — and
headers_for_a_record_the_tree_does_not_know_serve_nothingpins the outcome soit is recorded rather than silent.
getblocktests were built from a record alone. They now seed thetree too, through a
seed_blockhelper that does whatapply_blockdoes. Arecord on its own was never a node's state; those fixtures were asking
getblockto answer from half of it.Mutation audit
record_for_hashdrops the header spliceheader_recorddoes not read the header off the tree nodeapplied_block_recordstores the header againfrom_block_bytesstores the header againBaseline and restored green across all 14 targets.
Two of these are worth naming.
applied_block_recordstoring the header againis caught by
applied_block_record_matches_rpc_constructors, which predatesthis change: it asserts the node's builder and the rpc constructors agree, and
now that both must agree on no header it guards the memory claim on the node
side without having been written for it.
Un-boxing is caught only by the
size_ofassertion, which is the point ofhaving one — it compiles, passes every behavioural test, and silently hands 80
bytes per block back to the heap.
Verification
cargo test -p bitcoin-rs-rpc -p bitcoin-rs-node --no-default-features --features bitcoin-rs-node/fjall --no-fail-fast— 14/14 targets greencargo fmt --check— cleancargo clippy ... -- -D warnings— cleanNot in this change
record_for_hashstep 2 and therest.rsfallback stay. The evidence saysthey are unreachable in production; acting on that is its own claim.
BlockTree's unboundedSlab. Nothing removes a node, and the tree is nowthe single source of every header — which raises the value of that candidate
rather than lowering it.
Full write-up:
docs/benchmarks/block-record-footprint.md.Second commit: the ordering is enforced, not just argued (
e9502b6)Everything above rests on one ordering — the header is in the tree before the
record is in the log — and nothing checked it. In Bitcoin Core that state is not
representable at all:
CBlockIndexholds the header and the payload facts inone structure, so there is no such thing as a record without an index entry. Here
they are two structures held in step by code discipline.
The push site now asserts it:
The tree lock is free there —
applied_header_tipreleases its write guardbefore returning — and the check is one hash-table lookup, compiled out of
release builds.
It is not a lone test. Moving the record push above the tree insert — exactly
the mistake it defends against — fails 52 node tests, each on this assertion
naming the block that would have lost its header. Every node test that applies a
block now exercises the invariant.
One part of the argument needed checking, not assuming
A reorg calls
invalidate_subtree. Hadlookupfiltered on node status, aninvalidated block's header would have become unreachable while its record was
still in the log — a hole straight through the safety argument.
lookup(chain/src/tree.rs:146-154) matches on hash alone, andby_hashisonly ever
insert_uniqued (:615), so a hash resolves for the life of theprocess whatever happens to the branch it is on. Reorged-out blocks in fact
answer better than before: the record is popped on disconnect, but the tree
node stays, so
getblockheaderstill serves a header.On Bitcoin Core
For the record, since this change is about removing something Core does not
duplicate either.
CBlockIndexstores the header fields as members andreconstructs the header from them:
getblockheadercallsLookupBlockIndex(hash)and reads the fields off theindex — never from disk, which is why it works on pruned blocks. That is the
shape this change moves toward: the in-memory block index as the single
authority for headers.
Two honest gaps remain. Core keeps header and payload facts in one structure
where this codebase now has two — merging them is the Core-shaped end state and
would make the invariant above unrepresentable rather than asserted. And Core
does not store
hashPrevBlockat all, deriving it frompprev;BlockTreeNodestores both its own
hashandheader.prev_blockhash, which is 32 redundantbytes per node. Both are separate candidates.